-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: use new test-fixtures path #880
Conversation
@@ -17,6 +17,7 @@ | |||
"devDependencies": { | |||
"@typescript-eslint/eslint-plugin": "^5.40.0", | |||
"@typescript-eslint/parser": "^5.40.0", | |||
"axe-test-fixtures": "github:dequelabs/axe-test-fixtures#v1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't pinned to v1
so wasn't getting the path update. Also not sure why it was a dependency so made it a dev dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, please have a look
@@ -619,7 +616,7 @@ describe('@axe-core/webdriverjs', () => { | |||
}); | |||
|
|||
it('with chaining include and exclude', async () => { | |||
await driver.get(`${addr}/context.html`); | |||
await driver.get(`${addr}/context-include-exclude.html`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't really prove anything. If you only had the .includes the assertions would still pass because the excludes aren't nested in an include. Not sure that needs to be fixed in this PR, but something should be done about it IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Noticed that too. This would fix it dequelabs/axe-test-fixtures#28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Use the new exported
fixturesPath
fromaxe-test-fixtures
and remove all the symlinks toexternal
and allfixtures
directories.